fix #25678: return matters for generated functions #40778

simeonschaub · 2021-05-10T20:34:59Z

Just explicitly check for CodeInfo objects when doing the
interpolation of the generated parts. I initially tried a more
complicated version of this, which avoided doing this check if there was
more than one generated part, but that's probably not necessary here.

JeffBezanson · 2021-05-14T19:47:23Z

I think we should only apply this at the top-level, not recursively, so it's still possible to splice in a CodeInfo (there may not be any use case for that, but better to allow it). E.g.:

--- a/base/expr.jl
+++ b/base/expr.jl
@@ -434,7 +434,9 @@ macro generated(f)
                          Expr(:block,
                               lno,
                               Expr(:if, Expr(:generated),
-                                   body,
+                                   Expr(:block,
+                                        :(local tmp = $body),
+                                        :(if tmp isa Core.CodeInfo; return tmp; else tmp; end)),
                                    Expr(:block,
                                         Expr(:meta, :generated_only),

simeonschaub · 2021-05-19T14:26:06Z

I did think a while about how this should best be handled, but somehow that didn't occur to me. Yours seems like the better solution.

Just explicitely check for `CodeInfo` objects and use an explicit `return` in this case inside the `@generated` macro. Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>

vtjnash · 2021-05-19T15:37:27Z

Still seems rather buggy that lowering can tell whether the return keyword was used, when these appear to be redundant expressions?

simeonschaub · 2021-05-19T15:43:47Z

I didn't actually know about this, but the fact that you can just use if @generated anywhere in a function and it's basically just an interpolation if the function is called as generated is a pretty cool feature I think.
Or are you saying that lowering should just remove any explicit returns in the @generated() == true part? That might be quite nontrivial and once you understand what if @generated is doing, I think the current behavior is kind of intuitive.

simeonschaub · 2021-06-22T15:29:14Z

Bump

simeonschaub · 2021-07-24T08:15:53Z

This just came up again, is this OK to merge?

Just explicitely check for `CodeInfo` objects and use an explicit `return` in this case inside the `@generated` macro. Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com> (cherry picked from commit 92c84bf)

devmotion · 2022-01-14T00:06:08Z

base/expr.jl

+                                   # https://github.com/JuliaLang/julia/issues/25678
+                                   Expr(:block,
+                                        :(local tmp = $body),
+                                        :(if tmp isa Core.CodeInfo; return tmp; else tmp; end)),


@simeonschaub I guess generally it's a bad idea to name a module Core but this caused an error in Turing: TuringLang/Turing.jl#1756 (comment) and discussion in the issue. I think this could be fixed by interpolating Core.CodeInfo. I could prepare a PR (my first to base actually 😮) if you consider this to be a bug that should be fixed in base (I'll submit a PR to Turing in any case that renames the offending module).

That's funny, I was just staring at code generated by this today and was wondering whether having a call to getproperty in here was the right thing! It's generally more idiomatic to handle this as $(GlobalRef(Core, :CodeInfo)) instead of just interpolating the object though since that's what lowering would generate, but that sounds like the right thing to do.

Ref: #43823

…g#40778) Just explicitely check for `CodeInfo` objects and use an explicit `return` in this case inside the `@generated` macro. Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>

Just explicitely check for `CodeInfo` objects and use an explicit `return` in this case inside the `@generated` macro. Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com> (cherry picked from commit 92c84bf)

Expose the demanded world to the GeneratedFunctionStub caller, for users such as Cassette. If this argument is used, the uesr must return a CodeInfo with the min/max world field set correctly. Make the internal representation a tiny bit more compact also, removing a little bit of unnecessary metadata. Remove support for returning `body isa CodeInfo` via this wrapper, since it is impossible to return a correct object via the GeneratedFunctionStub since it strips off the world argument, which is required for it to do so. This also removes support for not inferring these fully (expand_early=false). Also answer method lookup queries about the future correctly, by refusing to answer them. This helps keeps execution correct as methods get added to the system asynchronously. This reverts "fix #25678: return matters for generated functions (#40778)" (commit 92c84bf), since this is no longer sensible to return here anyways, so it is no longer permitted or supported by this macro. Fixes various issues where we failed to specify the correct world.

Expose the demanded world to the GeneratedFunctionStub caller, for users such as Cassette. If this argument is used, the user must return a CodeInfo with the min/max world field set correctly. Make the internal representation a tiny bit more compact also, removing a little bit of unnecessary metadata. Remove support for returning `body isa CodeInfo` via this wrapper, since it is impossible to return a correct object via the GeneratedFunctionStub since it strips off the world argument, which is required for it to do so. This also removes support for not inferring these fully (expand_early=false). Also answer method lookup queries about the future correctly, by refusing to answer them. This helps keeps execution correct as methods get added to the system asynchronously. This reverts "fix #25678: return matters for generated functions (#40778)" (commit 92c84bf), since this is no longer sensible to return here anyways, so it is no longer permitted or supported by this macro. Fixes various issues where we failed to specify the correct world.

Expose the demanded world to the GeneratedFunctionStub caller, for users such as Cassette. If this argument is used, the user must return a CodeInfo with the min/max world field set correctly. Make the internal representation a tiny bit more compact also, removing a little bit of unnecessary metadata. Remove support for returning `body isa CodeInfo` via this wrapper, since it is impossible to return a correct object via the GeneratedFunctionStub since it strips off the world argument, which is required for it to do so. This also removes support for not inferring these fully (expand_early=false). Also answer method lookup queries about the future correctly, by refusing to answer them. This helps keeps execution correct as methods get added to the system asynchronously. This reverts "fix #25678: return matters for generated functions (#40778)" (commit 92c84bf), since this is no longer sensible to return here anyways, so it is no longer permitted or supported by this macro. Fixes various issues where we failed to specify the correct world. Note that the passed world may be `typemax(UInt)`, which demands that the generator must return code that is unspecialized (guaranteed to run correctly in any world). Co-authored-by: Jameson Nash <vtjnash@gmail.com>

Expose the demanded world to the GeneratedFunctionStub caller, for users such as Cassette. If this argument is used, the user must return a CodeInfo with the min/max world field set correctly. Make the internal representation a tiny bit more compact also, removing a little bit of unnecessary metadata. Remove support for returning `body isa CodeInfo` via this wrapper, since it is impossible to return a correct object via the GeneratedFunctionStub since it strips off the world argument, which is required for it to do so. This also removes support for not inferring these fully (expand_early=false). Also answer method lookup queries about the future correctly, by refusing to answer them. This helps keeps execution correct as methods get added to the system asynchronously. This reverts "fix JuliaLang#25678: return matters for generated functions (JuliaLang#40778)" (commit 92c84bf), since this is no longer sensible to return here anyways, so it is no longer permitted or supported by this macro. Fixes various issues where we failed to specify the correct world. Note that the passed world may be `typemax(UInt)`, which demands that the generator must return code that is unspecialized (guaranteed to run correctly in any world). Co-authored-by: Jameson Nash <vtjnash@gmail.com>

simeonschaub added the compiler:lowering Syntax lowering (compiler front end, 2nd stage) label May 10, 2021

simeonschaub force-pushed the sds/fix_25678 branch from db94c24 to 4194c8c Compare May 10, 2021 21:06

JeffBezanson self-assigned this May 14, 2021

fix #25678: return matters for generated functions

12cce25

Just explicitely check for `CodeInfo` objects and use an explicit `return` in this case inside the `@generated` macro. Co-authored-by: Jeff Bezanson <jeff.bezanson@gmail.com>

simeonschaub force-pushed the sds/fix_25678 branch from 4194c8c to 12cce25 Compare May 19, 2021 14:37

simeonschaub requested a review from JeffBezanson May 19, 2021 21:49

Merge branch 'master' into sds/fix_25678

c09b0eb

Merge remote-tracking branch 'origin/master' into sds/fix_25678

575b616

JeffBezanson merged commit 92c84bf into master Aug 24, 2021

JeffBezanson deleted the sds/fix_25678 branch August 24, 2021 18:19

simeonschaub added backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Nov 22, 2021

KristofferC mentioned this pull request Dec 2, 2021

release-1.6: Backports for 1.6.5 #43152

Merged

29 tasks

KristofferC removed the backport 1.6 Change should be backported to release-1.6 label Dec 11, 2021

devmotion reviewed Jan 14, 2022

View reviewed changes

KristofferC removed the backport 1.7 label Feb 15, 2022

wazizian mentioned this pull request Mar 27, 2022

Issue with IRTools after #40778 #44768

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #25678: return matters for generated functions #40778

fix #25678: return matters for generated functions #40778

simeonschaub commented May 10, 2021

JeffBezanson commented May 14, 2021

simeonschaub commented May 19, 2021

vtjnash commented May 19, 2021

simeonschaub commented May 19, 2021 •

edited

Loading

simeonschaub commented Jun 22, 2021

simeonschaub commented Jul 24, 2021

devmotion Jan 14, 2022 •

edited

Loading

simeonschaub Jan 14, 2022 •

edited

Loading

devmotion Jan 15, 2022

fix #25678: return matters for generated functions #40778

fix #25678: return matters for generated functions #40778

Conversation

simeonschaub commented May 10, 2021

JeffBezanson commented May 14, 2021

simeonschaub commented May 19, 2021

vtjnash commented May 19, 2021

simeonschaub commented May 19, 2021 • edited Loading

simeonschaub commented Jun 22, 2021

simeonschaub commented Jul 24, 2021

devmotion Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

simeonschaub Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

devmotion Jan 15, 2022

Choose a reason for hiding this comment

simeonschaub commented May 19, 2021 •

edited

Loading

devmotion Jan 14, 2022 •

edited

Loading

simeonschaub Jan 14, 2022 •

edited

Loading